-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add TryFrom
to convert repr to enum
#300
Conversation
5610f38
to
fa44984
Compare
We could also support some |
15fa1fc
to
7b54545
Compare
7b54545
to
49f7eaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ModProg good job! Thanks 👍
Overall design is solid and thoughtful. The implementation is also good, despite few bikesheddings described below.
We could also support some
#[try_from(<type>)]
for types whose definition is a superset to the repr type.
Definitely not as a part of this PR. Let's sketch and discuss the design of it in #146.
@ModProg @JelteF yet another thing that I think of: we will definitely want to support this case in the future: #[derive(TryFrom)]
enum Foo {
#[try_from(u128)] // generates `impl TryFrom<u128> for Foo`
Bar(u8),
} While at the same time, in other situations we would like to have something like this: #[derive(TryFrom)]
#[try_from(u128)] // generates `impl TryFrom<u128> for Foo`
#[repr(u8)]
enum Foo {
Bar,
} So, we need a way to differentiate a And this question touches not only #[derive(Into)] // generates `impl From<Foo> for u8`
#[repr(u8)]
enum Foo {
Bar = 35,
} And #[derive(TryInto)]
#[try_into(u8)] // generates `impl TryFrom<Foo> for u8`
#[repr(u128)]
enum Foo {
Bar = 35,
} To disambiguate them clearly, I propose to use #[derive(Into, TryFrom)]
#[into(repr)] // generates `impl From<Foo> for u8`
#[try_from(repr)] // generates `impl TryFrom<u8> for Foo`
#[repr(u8)]
enum Foo1 {
Bar,
}
#[derive(TryFrom, TryInto)]
#[try_from(repr(u128))] // generates `impl TryFrom<u128> for Foo`
#[try_into(repr(u8))] // generates `impl TryFrom<Foo> for u8`
#[repr(u16)]
enum Foo2 {
Bar,
} Does it make sense? |
@tyranron, looks sensible. I'll implement support for the repr syntax for now, and will error on it not specified. |
@tyranron how would you expect impl TryFrom<$type> for $enum {
fn try_from(value: $type) -> Result<Self> {
let value: $actual_repr = value.try_into().map_err(|e| TryFromError::message(e.to_string())?;
match value {
$actual_implementation
}
}
} |
@ModProg as a regular forwarding implementation, like you've pointed, yes. But as I've mentioned before, let's not making it as a part of this PR. Better implement as a separate one after merging this. |
7a35573
to
9a63f3e
Compare
9a63f3e
to
21b5faf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me (apart from the few small comments I left). @tyranron did you have any more feedback.
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
@JelteF oh, and please, change the required CI jobs in project settings to reflect MSRV 1.65.0 -> MSRV 1.72.0 migration. Otherwise, CI won't pass. Thanks! |
The MSRV bump in JelteF#300 is unnecessary. The 1.72 minimum is only needed for tests, but the actual implementation depends on 1.65.
## Synopsis The MSRV bump in #300 is unnecessary. The 1.72 minimum is only needed for tests, but the actual implementation depends on 1.65. ## Solution - Lower MSRV from 1.72 to 1.65. - Omit tests requiring higher MSRV in `msrv` CI job. - Describe "MSRV policy" in README. Co-authored-by: Kai Ren <tyranron@gmail.com>
## Synopsis The MSRV bump in JelteF/derive_more#300 is unnecessary. The 1.72 minimum is only needed for tests, but the actual implementation depends on 1.65. ## Solution - Lower MSRV from 1.72 to 1.65. - Omit tests requiring higher MSRV in `msrv` CI job. - Describe "MSRV policy" in README. Co-authored-by: Kai Ren <tyranron@gmail.com>
Resolves #146
Synopsis
Conversion from the repr integer to an enum.
Solution
A
TryFrom
derive macro implementingTryFrom<integertype> for Enum
.Checklist